Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[windows exporter] Clarify the use of use_api in docs. #6603

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

ptodev
Copy link
Contributor

@ptodev ptodev commented Mar 5, 2024

I tried to add as much information as possible, based on what I understand from the upstream PR. It would be good if someone with knowledge of the Windows exporter double checks my wording.

@ptodev ptodev requested a review from mattdurham March 5, 2024 12:19
@ptodev ptodev requested a review from clayton-cornell as a code owner March 5, 2024 12:19
@mattdurham
Copy link
Collaborator

From a windows perspective looks good, @clayton-cornell can add context around the verbiage itself.

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Mar 5, 2024
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions to expand the text just a little


The `where_clause` argument can be used to limit the response to the services you specify, reducing the size of the response.
If `use_api` is enabled, 'where_clause' won't be effective.

The Windows API is more performant than WMI. Set `use_api` to `true` in situations when the WMI takes too long to get the service information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The Windows API is more performant than WMI. Set `use_api` to `true` in situations when the WMI takes too long to get the service information.
The WMI is slow to query data, but it provides detailed results. The Windows API is fast but you must make several API calls to get the same amount of information. If speed is important and the WMI takes too long to retrieve information, you can set `use_api` to `true` to use the API to query your data.

I gave it a go at tweaking the text here a little to add a little more contextual info and details. Just saying the API is more performant doesn't tell you very much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I think the extra text feels a bit vague and creates more questions than answers.

The WMI is slow to query data, but it provides detailed results.

Why is it more detailed?

The Windows API is fast but you must make several API calls to get the same amount of information.

Do the number of API calls matter? And I'm not sure where we got this information from?

I'm personally not an expert on this exporter so I'd prefer not to add too much information that I'm not sure about 😅


The `where_clause` argument can be used to limit the response to the services you specify, reducing the size of the response.
If `use_api` is enabled, 'where_clause' won't be effective.

The Windows API is more performant than WMI. Set `use_api` to `true` in situations when the WMI takes too long to get the service information.
Setting `use_api` to `true` does have a few disadvantages compared to using WMI:
* WMI queries in `where_clause` won't work.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any alternative here? The second bullet provides an alternate solution.. so I'm wondering what happens in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there are alternatives. But I also don't think it's worth elaborating too much on this, since it seems natural that the "WMI queries" feature won't work when "WMI" is disabled.

The second bullet point is not related to WMI queries IIUC.

Copy link
Contributor

github-actions bot commented Apr 5, 2024

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@github-actions github-actions bot added the needs-attention An issue or PR has been sitting around and needs attention. label Apr 5, 2024
@rfratto rfratto added variant/static Related to Grafana Agent Static. variant/flow Relatd to Grafana Agent Flow. labels Apr 9, 2024
@github-actions github-actions bot removed the needs-attention An issue or PR has been sitting around and needs attention. label Apr 10, 2024
Copy link
Contributor

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@github-actions github-actions bot added the needs-attention An issue or PR has been sitting around and needs attention. label May 11, 2024
@clayton-cornell
Copy link
Contributor

@mattdurham @ptodev Is this still valid? Can it be merged?

@mattdurham
Copy link
Collaborator

I am good with this, looks like @ptodev has a few doc changes requested?

@github-actions github-actions bot removed the needs-attention An issue or PR has been sitting around and needs attention. label Jun 4, 2024
@ptodev ptodev force-pushed the ptodev/clarify-use-api branch from fea3554 to f028491 Compare June 4, 2024 11:32
Copy link
Contributor

github-actions bot commented Jul 5, 2024

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@github-actions github-actions bot added the needs-attention An issue or PR has been sitting around and needs attention. label Jul 5, 2024
@clayton-cornell clayton-cornell added backport release-v0.43 and removed needs-attention An issue or PR has been sitting around and needs attention. backport release-v0.43 labels Oct 16, 2024
@clayton-cornell clayton-cornell enabled auto-merge (squash) October 16, 2024 00:27
@clayton-cornell clayton-cornell merged commit 69e7872 into main Oct 16, 2024
10 of 11 checks passed
@clayton-cornell clayton-cornell deleted the ptodev/clarify-use-api branch October 16, 2024 00:34
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Nov 16, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos variant/flow Relatd to Grafana Agent Flow. variant/static Related to Grafana Agent Static.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants